-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Workplace Search] Convert Sources pages to new page template (+ personal dashboard) #102592
Conversation
- Remove references to enterpriseSearchLayout CSS (which will be removed in an upcoming PR) - Prefer to lean more heavily on default EuiPage props/CSS/etc. - Handle conditional sidebar logic in this layout rather than passing it in as a prop - Update props & DRY concerns to more closely match WorkplaceSearchPageTemplate - e.g. isLoading & pageChrome (mostly for document titles) - make FlashMessage and readOnlyMode work OOTB w/o props)
+ update PrivateSourcesSidebar to use EuiSIdeNav
- removing wrapping layouts, flash messages, chrome/telemetry
- Remove license callout, page header, and page chrome/telemetry - NOTE: The early page isLoading behavior (lines 51-) is required to prevent a flash of a completely empty page (instead of preserving the layout/side nav while loading). We cannot let the page fall through to the route because some routes are conditionally rendered based on isCustomSource. - FWIW: App Search has a similar isLoading early return with its Engine sub nav, and also a similar AnalyticsLayout for DRYing out repeated concerns/UI elements within Analytics subroutes.
- Mostly removing isLoading tests - NOTE: Schema page could *possibly* use the new isEmptyState/emptyState page template props, but would need some layout reshuffling
- Opted to give these pages their own conditional layout logic - this could possibly be DRY'd out - There is possibly extra cleanup here on this file that could have been done (e.g. empty state, titles, etc.) in light of the new templates - but I didn't want to spend extra time here and went with creating as few diffs as possible
…page templates + fix Link to EuiButtonTo on Organization Sources view
+ add related KibanaLogic branch coverage
...terprise_search/public/applications/workplace_search/views/content_sources/source_router.tsx
Show resolved
Hide resolved
...arch/public/applications/workplace_search/views/content_sources/components/source_layout.tsx
Show resolved
Hide resolved
...arch/public/applications/workplace_search/views/content_sources/components/schema/schema.tsx
Show resolved
Hide resolved
...pplications/workplace_search/views/content_sources/components/add_source/add_source_list.tsx
Show resolved
Hide resolved
...pplications/workplace_search/views/content_sources/components/add_source/add_source_list.tsx
Show resolved
Hide resolved
...search/public/applications/workplace_search/views/account_settings/account_settings.test.tsx
Show resolved
Hide resolved
x-pack/plugins/enterprise_search/public/applications/shared/kibana/kibana_logic.test.ts
Show resolved
Hide resolved
...ns/workplace_search/components/layout/personal_dashboard_sidebar/private_sources_sidebar.tsx
Show resolved
Hide resolved
...s/workplace_search/components/layout/personal_dashboard_layout/personal_dashboard_layout.tsx
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
x-pack/plugins/enterprise_search/public/applications/workplace_search/index.tsx
Show resolved
Hide resolved
...arch/public/applications/workplace_search/views/content_sources/components/schema/schema.tsx
Show resolved
Hide resolved
...e_search/public/applications/workplace_search/views/content_sources/organization_sources.tsx
Show resolved
Hide resolved
x-pack/plugins/enterprise_search/public/applications/shared/kibana/kibana_logic.test.ts
Show resolved
Hide resolved
...search/public/applications/workplace_search/views/account_settings/account_settings.test.tsx
Show resolved
Hide resolved
x-pack/plugins/enterprise_search/public/applications/shared/kibana/kibana_logic.test.ts
Show resolved
Hide resolved
x-pack/plugins/enterprise_search/public/applications/shared/kibana/kibana_logic.test.ts
Show resolved
Hide resolved
x-pack/plugins/enterprise_search/public/applications/shared/kibana/kibana_logic.test.ts
Show resolved
Hide resolved
@constancecchen I finished the QA and everything except already posted comments looks great! Thank you so much for handling this update! Looking forward to the last PR! 🎉 |
…crumbs - `exact` is required to make the parent schemas/ not gobble schema/{errorId} - added bonus breadcrumb for nicer schema navigation UX - No tests need to update AFAICT
Thanks so much for the super thorough QA Vadim!!! Just to confirm, no issues on the personal dashboard either right? If not, I guess we get to wait for EUI! 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm, no issues on the personal dashboard either right?
Yep, no issues!
Approving since all comments are resolved. 💃
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
💔 Backport failed
To backport manually run: |
…onal dashboard) (elastic#102592) * Refactor PersonalDashboardLayout to more closely match new page template - Remove references to enterpriseSearchLayout CSS (which will be removed in an upcoming PR) - Prefer to lean more heavily on default EuiPage props/CSS/etc. - Handle conditional sidebar logic in this layout rather than passing it in as a prop - Update props & DRY concerns to more closely match WorkplaceSearchPageTemplate - e.g. isLoading & pageChrome (mostly for document titles) - make FlashMessage and readOnlyMode work OOTB w/o props) * Convert Source subnav to EuiSideNav format + update PrivateSourcesSidebar to use EuiSIdeNav * Update routers - removing wrapping layouts, flash messages, chrome/telemetry * Refactor SourceRouter into shared layout component - Remove license callout, page header, and page chrome/telemetry - NOTE: The early page isLoading behavior (lines 51-) is required to prevent a flash of a completely empty page (instead of preserving the layout/side nav while loading). We cannot let the page fall through to the route because some routes are conditionally rendered based on isCustomSource. - FWIW: App Search has a similar isLoading early return with its Engine sub nav, and also a similar AnalyticsLayout for DRYing out repeated concerns/UI elements within Analytics subroutes. * Convert all single source views to new page template - Mostly removing isLoading tests - NOTE: Schema page could *possibly* use the new isEmptyState/emptyState page template props, but would need some layout reshuffling * Convert Add Source pages to conditional page templates - Opted to give these pages their own conditional layout logic - this could possibly be DRY'd out - There is possibly extra cleanup here on this file that could have been done (e.g. empty state, titles, etc.) in light of the new templates - but I didn't want to spend extra time here and went with creating as few diffs as possible * Convert separate Organization Sources & Private Sources views to new page templates + fix Link to EuiButtonTo on Organization Sources view * Update Account Settings with personal layout + write tests + add related KibanaLogic branch coverage * [UX feedback] Do not render page headers while loading on Overview & Sources pages * [PR feedback] Breadcrumb errors/fallbacks * [Proposal] Update schema errors routing to better work with nav/breadcrumbs - `exact` is required to make the parent schemas/ not gobble schema/{errorId} - added bonus breadcrumb for nicer schema navigation UX - No tests need to update AFAICT * Ignore Typescript error on soon-to-come EUI prop # Conflicts: # x-pack/plugins/enterprise_search/public/applications/workplace_search/views/content_sources/components/source_settings.tsx
…onal dashboard) (#102592) (#102843) * Refactor PersonalDashboardLayout to more closely match new page template - Remove references to enterpriseSearchLayout CSS (which will be removed in an upcoming PR) - Prefer to lean more heavily on default EuiPage props/CSS/etc. - Handle conditional sidebar logic in this layout rather than passing it in as a prop - Update props & DRY concerns to more closely match WorkplaceSearchPageTemplate - e.g. isLoading & pageChrome (mostly for document titles) - make FlashMessage and readOnlyMode work OOTB w/o props) * Convert Source subnav to EuiSideNav format + update PrivateSourcesSidebar to use EuiSIdeNav * Update routers - removing wrapping layouts, flash messages, chrome/telemetry * Refactor SourceRouter into shared layout component - Remove license callout, page header, and page chrome/telemetry - NOTE: The early page isLoading behavior (lines 51-) is required to prevent a flash of a completely empty page (instead of preserving the layout/side nav while loading). We cannot let the page fall through to the route because some routes are conditionally rendered based on isCustomSource. - FWIW: App Search has a similar isLoading early return with its Engine sub nav, and also a similar AnalyticsLayout for DRYing out repeated concerns/UI elements within Analytics subroutes. * Convert all single source views to new page template - Mostly removing isLoading tests - NOTE: Schema page could *possibly* use the new isEmptyState/emptyState page template props, but would need some layout reshuffling * Convert Add Source pages to conditional page templates - Opted to give these pages their own conditional layout logic - this could possibly be DRY'd out - There is possibly extra cleanup here on this file that could have been done (e.g. empty state, titles, etc.) in light of the new templates - but I didn't want to spend extra time here and went with creating as few diffs as possible * Convert separate Organization Sources & Private Sources views to new page templates + fix Link to EuiButtonTo on Organization Sources view * Update Account Settings with personal layout + write tests + add related KibanaLogic branch coverage * [UX feedback] Do not render page headers while loading on Overview & Sources pages * [PR feedback] Breadcrumb errors/fallbacks * [Proposal] Update schema errors routing to better work with nav/breadcrumbs - `exact` is required to make the parent schemas/ not gobble schema/{errorId} - added bonus breadcrumb for nicer schema navigation UX - No tests need to update AFAICT * Ignore Typescript error on soon-to-come EUI prop # Conflicts: # x-pack/plugins/enterprise_search/public/applications/workplace_search/views/content_sources/components/source_settings.tsx
Summary
Follow up to #102170 - this is the last PR for converting a chunk of Workplace Search views to the new KibanaPageTemplate. It's the largest and most complex one since Sources is already the most complex set of views, and requires accounting for the Personal Dashboard layout + updating that as well. As always, follow along by commit and disable whitespace diffs, etc.
Screencaps
Organization Sources
Personal Dashboard
(Not sure why this one spent so much time loading or had an error on the content sources page...maybe because I'd just added the slack connector?)
QA
Checklist